Skip to content

feat(ci): gate merges on maintainer approval via a required status check - #3176

Open
purp wants to merge 14 commits into
NVIDIA:mainfrom
purp:feat/core-approval-gate/purp
Open

feat(ci): gate merges on maintainer approval via a required status check#3176
purp wants to merge 14 commits into
NVIDIA:mainfrom
purp:feat/core-approval-gate/purp

Conversation

@purp

@purp purp commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds a OpenShell / Core Approval commit status that passes only when someone listed in MAINTAINERS.md has approved the pull request. Once registered on ruleset 13332227 it becomes the approval gate, which is what makes it safe to disable the CODEOWNERS wildcard in #3168.

The motivating problem is that every native GitHub mechanism enforcing who must approve also notifies them:

Mechanism Enforces Notifies Supports outside collaborators
CODEOWNERS yes yes yes
Ruleset required reviewers yes yes no (teams only)
Required status check yes no yes (the list is a file)

A required status check is the only primitive that gates a merge silently. That splits enforcement from notification: this PR is the enforcement half. Targeted notification (area labels, per-area rosters) follows separately.

A file-based approver list also solves the outside-collaborator problem — three of thirteen maintainers cannot join an NVIDIA org team, but can be listed in a file.

Related Issue

No issue required. This does not touch OpenShell platform code — it is maintenance of the project's own review machinery, and is non-breaking as merged. The gate only starts enforcing when the status context is added to ruleset 13332227, which is a separate manual step (see Rollout).

Changes

  • tasks/scripts/core_approval.py — stdlib-only helper. decide parses MAINTAINERS.md, folds the review list to each reviewer's latest decisive position, and prints state<TAB>description. diff renders the approver-set delta between two versions of the file.
  • tasks/scripts/core_approval_test.py — 17 tests. tasks/test.toml gains test:core-approval.
  • .github/workflows/core-approval.yml — publishes the status on pull_request_target, pull_request_review, merge_group, and workflow_dispatch.
  • .github/workflows/maintainers-change-alert.yml — comments the added/removed handles on PRs touching MAINTAINERS.md, so reviewers see the delta instead of diffing a markdown table.
  • .github/zizmor.yml — two dangerous-triggers suppressions for the new workflows.

Security invariants

  1. Never check out or execute PR head code. pull_request_target runs with a write-capable token; executing contributor code under it is the standard escalation. The checkout is pinned to ref: main with a sparse checkout of only the helper script and persist-credentials: false.
  2. MAINTAINERS.md is read from main, never the PR ref. Otherwise a contributor adds themselves in their own PR and self-approves.
  3. Fail closed. An unparseable or empty list fails. No error path can publish success.

No ${{ }} interpolation appears inside any run: block; every context value routes through env:.

Why this trigger set

pull_request does not re-fire when someone approves, so the check would go stale red forever. pull_request_review fires but its jobs are not auto-surfaced as PR checks, so the status is POSTed explicitly. Fork PRs get a read-only token under pull_request and cannot POST at all; pull_request_target and pull_request_review both run in base-repo context.

merge_group publishes success unconditionally. A merge group only forms after the PR already satisfied this gate, and approvals cannot change while an entry sits in the queue. Without this, a required check that never reports on the merge-group ref stalls every entry for the full check_response_timeout_minutes (60).

Testing

  • mise run pre-commit passes
  • Unit tests added — 17 tests covering the valid table, a reformatted table, a zero-handle table (fails closed), self-approval, non-maintainer approval, dismissal and change-request semantics, out-of-order review IDs, and description truncation at 140 characters
  • E2E tests added/updated — not applicable

There is no pre-merge dry run, by construction. pull_request_target runs the base branch's workflow definition, so this workflow cannot be exercised from its own PR. if: github.repository_owner == 'NVIDIA' also keeps it from running on forks. Behavior is covered by unit tests here and verified live after merge, before the check is made required.

Reviewer notes

  • No $GITHUB_ENV writes. An earlier revision passed the head SHA between steps via $GITHUB_ENV, which trips zizmor's github-env rule under pull_request_target and fails the code-scanning check. The workflow now uses step outputs (steps.publish.outputs.posted / head_sha), which carry the same values without granting later steps an attacker-shaped environment. Note that the Zizmor High report job is informational, but the SARIF it uploads feeds a separate Advanced Security zizmor check that does fail on new alerts.
  • Residual gap: on workflow_dispatch, no SHA is knowable until the PR lookup runs, so a failure strictly before that point publishes nothing and logs a warning. A failure after the lookup is covered by the head_sha step output. Every other event resolves the SHA from the event payload.
  • Stale-dismissal race: the ruleset sets dismiss_stale_reviews_on_push: true. A push and a dismissal can interleave, but the check re-runs on synchronize, so it converges.
  • Admin bypass is unchanged. Admins can bypass this check exactly as they can bypass today's CODEOWNERS gate. Reducing the admin surface is deliberately out of scope.
  • johnnygreco holds admin via openshell-codeowners but is absent from MAINTAINERS.md. Deferred; revisit when the team list is reconciled.

Rollout

Order matters — reversing steps 1–3 and 4 blocks every open PR.

  1. Merge this PR.
  2. Backfill open PRs via workflow_dispatch (pr_number is required; there is no sweep mode). PRs opened before the workflow existed otherwise sit at "Expected" forever once the check is required.
  3. Verify live: a fork PR posts a status; a non-maintainer approval stays red; a maintainer approval turns it green with no new push; a push returns it red; a PR adding a handle to MAINTAINERS.md cannot self-approve.
  4. Add context OpenShell / Core Approval to ruleset 13332227.
  5. Merge chore(codeowners): disable wildcard ownership rule #3168.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable) — not applicable, no platform code changed

@copy-pr-bot

copy-pr-bot Bot commented Sep 4, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

Comment thread .github/workflows/core-approval.yml Fixed
Comment thread .github/workflows/core-approval.yml Fixed
purp added 10 commits September 8, 2026 16:31
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
A checkout or API failure on a merge_group run left STATUS_SHA empty, so the
guard skipped and the required check sat at Expected until the queue timed out.

Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Writing to $GITHUB_ENV under pull_request_target trips zizmor's github-env
rule, which fails the code-scanning check. Step outputs carry the same
values without granting later steps an attacker-shaped environment, and
the resolved head SHA now also covers a workflow_dispatch run that fails
after the pull request lookup.

Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
Add a required check that passes only when someone listed in
MAINTAINERS.md has an approving review on the pull request. Unlike
CODEOWNERS and ruleset required reviewers, a status check enforces who
must approve without notifying anyone, and its approver list is a file,
so outside collaborators can be listed.

The job reports through its own exit code rather than a posted commit
status, so it needs no write token and no head SHA. It runs on
pull_request_review, and on merge_group because the queue waits for
required contexts to report on the merge group ref regardless of the
pull request result.

MAINTAINERS.md and the decision helper are read from main, never the
pull request ref, so a contributor cannot add themselves and
self-approve. The helper fails closed when no handles parse.

Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
@purp
purp force-pushed the feat/core-approval-gate/purp branch from b6102b0 to f92a2df Compare September 8, 2026 23:32

@purp purp left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor per comments.

Comment thread tasks/scripts/core_approval.py Outdated
Comment thread tasks/scripts/core_approval.py Outdated
Comment thread tasks/scripts/core_approval_test.py Outdated
Comment thread tasks/scripts/core_approval_test.py Outdated
Comment thread tasks/scripts/core_approval.py Outdated
Comment thread tasks/scripts/core_approval.py Outdated
Comment thread tasks/scripts/core_approval.py Outdated
Each CI tool now does one thing: check_maintainer_approval.py decides the
gate, alert_maintainer_change.py renders the approver-set delta. Neither
takes a verb argument.

The login pattern is duplicated so each tool stands alone; a test asserts
the two patterns stay identical.

Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

@purp purp left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address all comments.

Comment thread .github/workflows/maintainers-change-alert.yml Outdated
Comment thread .github/workflows/maintainers-change-alert.yml
Comment thread .github/workflows/maintainers-change-alert.yml Outdated
Comment thread tasks/scripts/alert_maintainer_change.py Outdated
Comment thread tasks/scripts/alert_maintainer_change.py Outdated
Comment thread tasks/scripts/alert_maintainer_change.py
Comment thread tasks/scripts/alert_maintainer_change.py
Comment thread tasks/scripts/alert_maintainer_change_test.py Outdated
Comment thread tasks/scripts/alert_maintainer_change_test.py Outdated
Comment thread tasks/scripts/check_maintainer_approval.py Outdated
The change alert now prints nothing when MAINTAINERS.md moves prose
without moving logins, so no comment is posted. When the updated file
yields no logins at all it still posts the warning, then fails the job.

The comment marker moves to a COMMENT_MARKER environment variable set in
the workflow and read by both the script and the comment lookup, so the
two can no longer drift.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
@purp
purp marked this pull request as ready for review September 9, 2026 15:32
@purp
purp requested review from a team, derekwaynecarr, mrunalp and sjenning as code owners September 9, 2026 15:32
@purp
purp requested review from elezar and pimlock September 9, 2026 15:32

@pimlock pimlock left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, the only concern I have is about ability for the opened PR to modify the workflow that enforces the check.

I added one option to close this gap, let me know WDYT.

on:
merge_group:
types: [checks_requested]
pull_request_review:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this trigger, it's possible to overwrite the contents of the job in the PR, right?
E.g. if I change it to run: true, it would run that and report the check as successful, so I think it would be possible to create a PR, override this check, approve by anybody and merge?
The merge action is limited to anyone with write permissions though, so maybe this is fine.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it looks like there maybe be a way to close this gap - we could add all the workflows to require codeowners approval, this way these files would be enforced by github (and consequently would ping everyone, but probably this is fine, as these changes are not as often?).

Something like

/.github/ @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr
/MAINTAINERS.md @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr
/tasks/scripts/*maintainer*.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a visualization on how it would fit in the process.

stateDiagram-v2
    [*] --> AwaitingCheck: PR opened

    AwaitingCheck --> Evaluating: Review submitted or dismissed
    Evaluating --> Blocked: No current maintainer approval
    Evaluating --> Blocked: Lookup or parsing fails
    Evaluating --> Approved: Current maintainer approval exists

    Blocked --> Evaluating: Review submitted or dismissed
    Approved --> Evaluating: Review submitted or dismissed

    Approved --> AwaitingCheck: New commit pushed
    Blocked --> AwaitingCheck: New commit pushed

    Approved --> AwaitingOtherRequirements
    AwaitingOtherRequirements --> Evaluating: Review submitted or dismissed
    AwaitingOtherRequirements --> AwaitingCheck: New commit pushed
    AwaitingOtherRequirements --> MergeQueue: All requirements pass and authorized user queues PR

    MergeQueue --> QueueEvaluation: merge_group event
    QueueEvaluation --> QueueBlocked: Approval check or other required checks fail
    QueueEvaluation --> Merged: Approval check and all other requirements pass
    Merged --> [*]

    note right of Evaluating
        Read maintainer list and helper from main.
        Fetch current reviews from GitHub.
    end note

    note right of AwaitingOtherRequirements
        Proposed scoped CODEOWNERS protection:
        changes to enforcement files require
        native codeowner approval.
        Ordinary PRs do not request codeowners.
    end note

    note right of AwaitingCheck
        This PR does not trigger the check
        on opening or pushing alone.
        A review event starts evaluation.
    end note
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that a lot. One moment, please ...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in a56320b

A workflow triggered by pull_request_review runs the pull request's copy
of the workflow file rather than the one on main, so a contributor could
rewrite the approval gate in the same pull request it is meant to block.
Verified on this pull request: the Maintainer Approval workflow ran from
the branch even though it does not exist on main.

Give the enforcement paths an explicit code owner so GitHub blocks the
merge natively. The tampered job would still run and still report green,
but the pull request cannot merge without a code owner's approval.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
purp added a commit to purp/OpenShell that referenced this pull request Sep 9, 2026
These rules named core-approval.yml and core_approval.py, which no longer
exist: NVIDIA#3176 renamed the workflow to core-maintainer.yml and split the
helper into check_maintainer_approval.py and alert_maintainer_change.py.
CODEOWNERS matches on literal paths, so the rules protected nothing.

NVIDIA#3176 carries the replacement, widened to all of .github/workflows/ and
.github/actions/ rather than the single gate workflow. A workflow
triggered by pull_request_review runs the pull request's own copy of the
workflow file, so any workflow, not just the gate, can claim a required
check context or ask for a broader token.

This branch is back to its original single purpose: scoping down the
wildcard rule.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>
@mrunalp, @sjenning, and @derekwaynecarr are maintainers but not members
of @NVIDIA/openshell-codeowners, and as outside collaborators they cannot
join it. Owning these paths by team alone would have meant no Red Hat
maintainer could approve a change to the merge gate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Jim Meyer <jimeyer@nvidia.com>

@elezar elezar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some commits have Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> attribution.

Comment thread .github/CODEOWNERS
Comment on lines +28 to +29
/tasks/scripts/check_maintainer_approval.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr
/tasks/scripts/alert_maintainer_change.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What sets these apart from other scripts? Should we add a folder for these so that they are clearly separated?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, should the test files be included too?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script is only for "convenience", correct? The core-maintainer.yml flow SHOULD still add maintainers / CODEOWNERS to the review already. Where they can review the diff.

My question is, does adding this add any value, or does it complicate the process and increase the possible attack surface?

# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# SPDX-License-Identifier: Apache-2.0

name: Maintainer Approval

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the YAML file be called maintainer-approval.yml?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants